Interpolation of RF predictions with cosZD, for homogeneous performance #1320

moralejo · 2024-11-27T09:55:58Z

This should solve the issue of RF "performance jumps" at high zenith angles, when the pointing goes through the middle points between training nodes. See #1317

NOTE: this is a different implementation of the interpolation approach proposed by @gabemery, see branch https://github.com/cta-observatory/cta-lstchain/tree/dl2_RF_interpolate)

If the interpolation option is activated we call the RF predictors twice, for each of the two closest MC training nodes, then interpolate (or extrapolate) the values to the actual telescope pointing for each event.

Currently the training sample pointings are obtained from the path to the training sample(provided via config file). A better solution would be to add to the .sav files an array with the zenith and azimuth values of the MC training nodes.

to avoid jumps when using RFs trained on a discrete set of pointings

codecov · 2024-11-27T10:10:21Z

Codecov Report

Attention: Patch coverage is 44.82759% with 48 lines in your changes missing coverage. Please review.

Project coverage is 73.30%. Comparing base (9aaa78b) to head (324a8c6).
Report is 41 commits behind head on main.

Files with missing lines	Patch %	Lines
lstchain/reco/dl1_to_dl2.py	25.00%	45 Missing ⚠️
lstchain/scripts/lstchain_dl1_to_dl2.py	88.88%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1320      +/-   ##
==========================================
- Coverage   73.52%   73.30%   -0.22%     
==========================================
  Files         134      134              
  Lines       14215    14308      +93     
==========================================
+ Hits        10451    10488      +37     
- Misses       3764     3820      +56

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

command-line options The interpolation can be switched on for the three reconstructions independently (energy, gammaness, direction) By default the correction is applied. If no MC DL1 training directory is provided, the path will be built from the path of the RF models. If directory does not exist the option is simply deactivated

vuillaut · 2024-11-28T12:16:45Z

Hi @moralejo
Thank you for making this implementation.
Early feedback:
would it be possible to add the training and testing nodes of the current production as an ascii file in the lstchain data files to allow for testing without necessarily being on the cluster?
(astropy tables can be dumped to ascii easily)
I completely agree these pointing nodes should be saved with the models during training and then loaded from there in the future.

moralejo · 2024-11-28T13:37:08Z

Hi @moralejo Thank you for making this implementation. Early feedback: would it be possible to add the training and testing nodes of the current production as an ascii file in the lstchain data files to allow for testing without necessarily being on the cluster? (astropy tables can be dumped to ascii easily)

Will do. What is better, just the zenith and azimuth values, or a list of the directory names?

vuillaut · 2024-11-28T14:06:03Z

Hi @moralejo Thank you for making this implementation. Early feedback: would it be possible to add the training and testing nodes of the current production as an ascii file in the lstchain data files to allow for testing without necessarily being on the cluster? (astropy tables can be dumped to ascii easily)

Will do. What is better, just the zenith and azimuth values, or a list of the directory names?

I think the table of values that you can use directly at inference would be great (with appropriate metadata such as the date of export).

(for RF interpolation)

Instead of closest angular distance we now take the closest nodes in alt_tel on the same side of the culmination (same sign of sin_az_tel)

Copilot reviewed 2 out of 3 changed files in this pull request and generated no suggestions.

Files not reviewed (1)

lstchain/data/lstchain_standard_config.json: Language not supported

Comments skipped due to low confidence (1)

lstchain/reco/dl1_to_dl2.py:215

The word 'dictionnary' is misspelled. It should be 'dictionary'.

config: dictionnary containing configuration

lstchain/data/lstchain_standard_config.json

lstchain/scripts/lstchain_dl1_to_dl2.py

moralejo · 2025-01-23T11:08:48Z

the training directions are saved as ascii files, produced automatically with the models. At inference this ascii file is loaded with the model to get training direction. But we could actually produce these ascii files manually for the current productions and add them to the models directories in the cluster to allow for interpolated inference already.

To save the pointing directions as ascii files, I'd recommend starting from astropy tables and save to ecsv

I have created the ECVS files for all directories of the new production, e.g.:
/fefs/aswg/data/models/AllSky/20240918_v0.10.12_allsky_nsb_tuning_0.00/dec_4822/training_dirs.ecsv
Please check if this is ok, I will change the code to use these (update: already done!).

Now the info is stored with the RF models in an ECVS file, and it is simply read from there

grown... Cast to numpy array of az and zd (solves error)

morcuended

It looks good to me. I've just left some modifications on the docstrings following the numpydocs standard so it is later properly render in the documentation.

lstchain/reco/dl1_to_dl2.py

lstchain/scripts/lstchain_dl1_to_dl2.py

lstchain/reco/dl1_to_dl2.py

…ormat)

moralejo · 2025-01-24T13:16:01Z

Thanks @morcuended !
@vuillaut about the way to store and retrieve the training node pointings: is that ok? Can we merge?

morcuended · 2025-01-24T13:21:19Z

about the way to store and retrieve the training node pointings: is that ok? Can we merge?

From my side this is fine. Maybe for the future productions this file should be produced by lstmcpipe?

One of these files, could even be included in lstchain test dataset for unit testing.

moralejo · 2025-01-24T13:32:38Z

about the way to store and retrieve the training node pointings: is that ok? Can we merge?

From my side this is fine. Maybe for the future productions this file should be produced by lstmcpipe?

yes, that is the idea.

One of these files, could even be included in lstchain test dataset for unit testing.

For this we would need to store also RFs trained with several pointings. Is that the case now?

morcuended

I left one last comment.

For this we would need to store also RFs trained with several pointings. Is that the case now?

No, right now dummy models are created from the test gamma MC, with single pointing

lstchain/reco/dl1_to_dl2.py

moralejo · 2025-01-27T14:32:19Z

I left one last comment.

For this we would need to store also RFs trained with several pointings. Is that the case now?

No, right now dummy models are created from the test gamma MC, with single pointing

Ok, then we cannot introduce a unit test now. This is more than two months old, we need to move forward...

moralejo added 2 commits November 27, 2024 10:47

New feature to perform interpolation of RF predictions vs. cos(zenith)

a2ef2f0

to avoid jumps when using RFs trained on a discrete set of pointings

Improved help message

908e6cf

moralejo mentioned this pull request Nov 27, 2024

Make RF response more homogeneous vs. zenith #1317

Open

moralejo added 11 commits November 27, 2024 16:04

Fixed various errors...

000f091

Add is_proba option for RF classifiers

19c6480

Proper treatment of predict_proba output

dba62a2

minor fix of matrix operations

d45ef90

Check RF type automatically - remove argument

d4a28db

Improved description

4b1c960

Change in treatment of missing dl1_training_dir case

309ab49

Fix building of dl1_training_dir from path to RF models

98a90ae

Typo

2acdf9f

Protection against MC training nodes with identical zenith

588a248

This comment was marked as resolved.

Sign in to view

This comment was marked as outdated.

Sign in to view

moralejo added 5 commits November 28, 2024 15:53

Preparations for better configuration of training pointings

2969a25

(for RF interpolation)

Moved function

32620d9

import glob

a65112b

Wrong variable scope!

9e37b7a

Better log message

80c0ffc

moralejo marked this pull request as ready for review November 29, 2024 14:58

Changed selection of pointing nodes for interpolation.

b0afdac

Instead of closest angular distance we now take the closest nodes in alt_tel on the same side of the culmination (same sign of sin_az_tel)

moralejo requested a review from vuillaut December 3, 2024 14:16

vuillaut requested a review from Copilot December 11, 2024 14:26

Copilot AI reviewed Dec 11, 2024

View reviewed changes

This comment was marked as resolved.

Sign in to view

vuillaut requested changes Jan 14, 2025

View reviewed changes

lstchain/data/lstchain_standard_config.json Outdated Show resolved Hide resolved

lstchain/scripts/lstchain_dl1_to_dl2.py Outdated Show resolved Hide resolved

This comment was marked as resolved.

Sign in to view

moralejo added 2 commits January 23, 2025 11:36

logger.warning => logger.info

c0d6a6b

warning => info

f2bfdc1

moralejo added 8 commits January 23, 2025 12:34

Changed approach to obtain the pointings used for theRF training

5bb6e4d

Now the info is stored with the RF models in an ECVS file, and it is simply read from there

Removed unused setting

b0dbbc7

Some simplifications

f5d519a

Unused import

c0d90b3

Simplification: use np.broadcast

9aabd2b

Simplification

2023bcf

Various simplifications

3c29005

Fix problem with RFs expecting the same feature names as when they were

43bb86e

grown... Cast to numpy array of az and zd (solves error)

moralejo requested review from vuillaut and morcuended January 23, 2025 15:35

Removed unneeded conversion to Series

9d1c1d3

morcuended previously approved these changes Jan 24, 2025

View reviewed changes

[skip ci] Apply suggestions from code review (use proper docstrings f…

12f875d

…ormat)

morcuended dismissed their stale review via 12f875d January 24, 2025 11:39

morcuended previously approved these changes Jan 24, 2025

View reviewed changes

morcuended reviewed Jan 27, 2025

View reviewed changes

lstchain/reco/dl1_to_dl2.py Outdated Show resolved Hide resolved

Remove default mutable argument

324a8c6

moralejo dismissed morcuended’s stale review via 324a8c6 January 27, 2025 14:28

morcuended approved these changes Jan 27, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Interpolation of RF predictions with cosZD, for homogeneous performance #1320

Interpolation of RF predictions with cosZD, for homogeneous performance #1320

moralejo commented Nov 27, 2024 •

edited

Loading

codecov bot commented Nov 27, 2024 •

edited

Loading

vuillaut commented Nov 28, 2024

moralejo commented Nov 28, 2024

vuillaut commented Nov 28, 2024

This comment was marked as resolved.

This comment was marked as outdated.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

moralejo commented Jan 23, 2025 •

edited

Loading

morcuended left a comment

moralejo commented Jan 24, 2025

morcuended commented Jan 24, 2025

moralejo commented Jan 24, 2025

morcuended left a comment

moralejo commented Jan 27, 2025

Interpolation of RF predictions with cosZD, for homogeneous performance #1320

Are you sure you want to change the base?

Interpolation of RF predictions with cosZD, for homogeneous performance #1320

Conversation

moralejo commented Nov 27, 2024 • edited Loading

codecov bot commented Nov 27, 2024 • edited Loading

Codecov Report

vuillaut commented Nov 28, 2024

moralejo commented Nov 28, 2024

vuillaut commented Nov 28, 2024

This comment was marked as resolved.

This comment was marked as outdated.

Choose a reason for hiding this comment

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

moralejo commented Jan 23, 2025 • edited Loading

morcuended left a comment

Choose a reason for hiding this comment

moralejo commented Jan 24, 2025

morcuended commented Jan 24, 2025

moralejo commented Jan 24, 2025

morcuended left a comment

Choose a reason for hiding this comment

moralejo commented Jan 27, 2025

moralejo commented Nov 27, 2024 •

edited

Loading

codecov bot commented Nov 27, 2024 •

edited

Loading

moralejo commented Jan 23, 2025 •

edited

Loading